Switch sr-arithmetic benchmarking to criterion by expenses · Pull Request #3902 · paritytech/substrate

expenses · 2019-10-24T07:05:27Z

As mentioned by @bkchr and @kianenigma in #3799, we should try and use criterion for benchmarking where possible. I wanted to replace all old #[bench] benchmarks with criterion in this pr, but unfortunately the subkey benchmarks aren't exposed, the node executor benchmarks require an unwanted amount of code duplication and the phragmen benchmarks are complied via a macro. I'm not sure it's worth it to try and convert these benchmarks at the moment.

I did rename the core/primitives benchmark file from benches.rs to bench.rs to be more consistent though.

substrate/subkey/src/vanity.rs

Lines 163 to 173 in eda5fe5

    
           #[cfg(feature = "bench")] 
        
           #[bench] 
        
           fn bench_paranoiac(b: &mut Bencher) { 
        
           	b.iter(|| generate_key("polk")); 
        
           } 
        
           #[cfg(feature = "bench")] 
        
           #[bench] 
        
           fn bench_not_paranoiac(b: &mut Bencher) { 
        
           	b.iter(|| generate_key("polk")); 
        
           }

substrate/node/executor/src/lib.rs

Lines 1215 to 1230 in eda5fe5

    
           #[cfg(feature = "benchmarks")] 
        
           mod benches { 
        
           	use super::*; 
        
           	use test::Bencher; 
        
           	#[bench] 
        
           	fn wasm_execute_block(b: &mut Bencher) { 
        
           		let (block1, block2) = blocks(); 
        
           		b.iter(|| { 
        
           			let mut t = new_test_ext(COMPACT_CODE, false); 
        
           			WasmExecutor::new().call(&mut t, "Core_execute_block", &block1.0).unwrap(); 
        
           			WasmExecutor::new().call(&mut t, "Core_execute_block", &block2.0).unwrap(); 
        
           		}); 
        
           	} 
        
           }

substrate/core/phragmen/benches/phragmen.rs

Lines 158 to 174 in eda5fe5

    
           macro_rules! phragmen_benches { 
        
           	($($name:ident: $tup:expr,)*) => { 
        
           	$( 
        
                   #[bench] 
        
                   fn $name(b: &mut Bencher) { 
        
           			let (v, n, t, e, eq_iter, eq_tol) = $tup; 
        
           			println!("----------------------"); 
        
           			println!( 
        
           				"++ Benchmark: {} Validators // {} Nominators // {} Edges-per-nominator // {} \ 
        
           				total edges // electing {} // Equalize: {} iterations -- {} tolerance", 
        
           				v, n, e, e * n, t, eq_iter, eq_tol, 
        
           			); 
        
           			do_phragmen(b, v, n, t, e, eq_iter, eq_tol); 
        
                   } 
        
               )* 
        
           	} 
        
           }

kianenigma · 2019-10-24T07:18:32Z

I'm not sure it's worth it to try and convert these benchmarks at the moment.

Indeed. My personal opinion is that: criterion is great but let's not refactor to it just for the sake of refactoring. Criterion is great when you want to pass complex data to the benchmark. This is something that #[bench] cannot do for you neatly and you ned up with ugly macros like phragmen benchmarks Notably, you can see the hashing benchmarks in which we want to benchmark over first fixed size and then dynamically linear growth etc. I see it rational to switch to criterion when you want to extend some benchmarks of some crate (or you plan to do) and then you realise test crate does not cut it for you.

from this perspective,

the phragmen can use criterion but at the time it was way too much hassle to set up criterion properly and I just pass my desired data with a macro. Nonetheless, the macro is just to do what criterion can also do so it does need a refactor at some point.
arithmatic can use criterion given but noting that we also want to probably pass more dynamic and sphisticated data into the benchmark. We can further discuss this
node/executor seem to be fine with #[bench]
subkey seems to be fine with #[bench]

of course, this is my personal opinion and how I would like to spend my time. You can go ahead and do more if you want. Nonetheless, thanks for the arithmetic benchmarks that would be very helpful now or in the future! :)

kianenigma

I think it can use criterion a bit more and remove almost-duplicated-code. Regarding the rest, I would be happier to approve this earlier and not jump into refactoring every single #[bench], but the author can decide otherwise. Thanks :)

bkchr · 2019-10-24T07:40:34Z

The reason I want to see this change #[bench] is unstable and requires a nightly compiler and I'm not sure they will ever stabilize it. :)

Ty for the pr! :)

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

kianenigma · 2019-10-24T15:58:59Z

+
+fn bench_subtraction(c: &mut Criterion) {
+	bench_op(c, "subtraction", |a, b| {
+		let _ = a.clone().sub(&b);


Suggested change

let _ = a.clone().sub(&b);

let _ = a.clone().sub(&b);

better to have let _ = .. consistently for all or none.

kianenigma · 2019-10-24T16:10:29Z

+			let a = random_big_uint(size as usize);
+			let b = random_big_uint(size as usize);
+
+			bencher.iter(|| op(&a, &b));


hmm I just ran it and being amazed of why division is as fast as addition: we don't do division p / q if the size of q is equal to p. Need to be reworked. I'd leave it up to you regarding how to bake it out of criterion, but ideally, given p with size limbs, we should chose a secondary random number between [2, size-1] and build a number of that size and divide them.

kianenigma

Needs a small rework of the division

kianenigma · 2019-10-27T12:40:44Z

@expenses done and review needed? please update the labels as needed :)

expenses · 2019-10-27T21:21:43Z

I don't think I can set labels, sorry.

kianenigma

looks good! CI is a bit overloaded now so will merge later.

expenses added 5 commits October 19, 2019 18:36

Change DefaultMaxDepth from 1024 to 32

72080f2

Merge branch 'master' of https://github.com/paritytech/substrate

6d5f98e

Merge branch 'master' of https://github.com/paritytech/substrate

113e38a

Merge branch 'master' of https://github.com/paritytech/substrate

45d3535

Switch sr-arithmetic benchmarking to criterion

ce0cae9

kianenigma reviewed Oct 24, 2019

View reviewed changes

Comment thread core/sr-arithmetic/benches/bench.rs Outdated

kianenigma reviewed Oct 24, 2019

View reviewed changes

Comment thread core/sr-arithmetic/benches/bench.rs Outdated

Comment thread core/sr-arithmetic/benches/bench.rs Outdated

kianenigma added the A0-please_review Pull request needs code review. label Oct 24, 2019

bkchr reviewed Oct 24, 2019

View reviewed changes

Comment thread core/sr-arithmetic/Cargo.toml Outdated

Comment thread core/sr-arithmetic/benches/bench.rs Outdated

expenses and others added 3 commits October 24, 2019 09:02

Update core/sr-arithmetic/Cargo.toml

59dc577

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Update core/sr-arithmetic/benches/bench.rs

4d05c62

Co-Authored-By: Bastian Köcher <bkchr@users.noreply.github.com>

Test on variable limb lengths

49faba4

gavofyork reviewed Oct 24, 2019

View reviewed changes

Comment thread core/sr-arithmetic/benches/bench.rs Outdated

Change license

f0130da

kianenigma reviewed Oct 24, 2019

View reviewed changes

kianenigma suggested changes Oct 24, 2019

View reviewed changes

gavofyork added A4-gotissues and removed A0-please_review Pull request needs code review. labels Oct 25, 2019

Rework division

da54522

expenses requested a review from kianenigma October 27, 2019 21:19

kianenigma added A1-onice and removed A4-gotissues labels Oct 28, 2019

kianenigma approved these changes Oct 28, 2019

View reviewed changes

bkchr merged commit 23c1956 into paritytech:master Oct 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switch sr-arithmetic benchmarking to criterion - #3902

Switch sr-arithmetic benchmarking to criterion#3902
bkchr merged 10 commits into
paritytech:masterfrom
expenses:sr-arithmetic-criterion

expenses commented Oct 24, 2019

Uh oh!

kianenigma commented Oct 24, 2019

Uh oh!

Uh oh!

kianenigma left a comment

Uh oh!

Uh oh!

Uh oh!

bkchr commented Oct 24, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kianenigma Oct 24, 2019

Uh oh!

kianenigma Oct 24, 2019

Uh oh!

kianenigma left a comment

Uh oh!

kianenigma commented Oct 27, 2019

Uh oh!

expenses commented Oct 27, 2019

Uh oh!

kianenigma left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	#[cfg(feature = "bench")]
	#[bench]
	fn bench_paranoiac(b: &mut Bencher) {
	b.iter(\|\| generate_key("polk"));
	}

	#[cfg(feature = "bench")]
	#[bench]
	fn bench_not_paranoiac(b: &mut Bencher) {
	b.iter(\|\| generate_key("polk"));
	}

	#[cfg(feature = "benchmarks")]
	mod benches {
	use super::*;
	use test::Bencher;

	#[bench]
	fn wasm_execute_block(b: &mut Bencher) {
	let (block1, block2) = blocks();

	b.iter(\|\| {
	let mut t = new_test_ext(COMPACT_CODE, false);
	WasmExecutor::new().call(&mut t, "Core_execute_block", &block1.0).unwrap();
	WasmExecutor::new().call(&mut t, "Core_execute_block", &block2.0).unwrap();
	});
	}
	}

	macro_rules! phragmen_benches {
	($($name:ident: $tup:expr,)*) => {
	$(
	#[bench]
	fn $name(b: &mut Bencher) {
	let (v, n, t, e, eq_iter, eq_tol) = $tup;
	println!("----------------------");
	println!(
	"++ Benchmark: {} Validators // {} Nominators // {} Edges-per-nominator // {} \
	total edges // electing {} // Equalize: {} iterations -- {} tolerance",
	v, n, e, e * n, t, eq_iter, eq_tol,
	);
	do_phragmen(b, v, n, t, e, eq_iter, eq_tol);
	}
	)*
	}
	}

Uh oh!

Conversation

expenses commented Oct 24, 2019

Uh oh!

kianenigma commented Oct 24, 2019

Uh oh!

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkchr commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kianenigma Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

kianenigma Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

kianenigma commented Oct 27, 2019

Uh oh!

expenses commented Oct 27, 2019

Uh oh!

kianenigma left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bkchr commented Oct 24, 2019 •

edited

Loading